@claude review

Claude finished @malinskibeniamin's task in 3m 27s —— View job

Code Review: Security page refactor

Tasks

• Gather context and understand PR
• Read CLAUDE.md for repo guidelines
• Review changed files
• Post comprehensive review feedback

Summary

Good overall refactor — moving from full-page routes to dialogs for create/edit flows is a cleaner UX pattern, and the shared AclsCard, SecurityTabsNav, and DescriptionWithHelp components help reduce duplication. A few bugs and quality gaps are worth addressing before merge.

🐛 Bugs

1. Broken E2E test: create-acls-button testId removed [user-management.spec.ts:72]

The test clicks create-acls-button and then asserts URL /security/acls/create, but the ACL create route was deleted and the button's testId was replaced with go-to-user-details-button in the confirmation modal (user-create.tsx:364). This test will fail.

// user-management.spec.ts:72-80 — references deleted route + removed testId
await page.getByTestId('create-acls-button').click();
await expect(page).toHaveURL(/\/security\/acls\/create/);

Similarly, acl.spec.ts and acl-page.ts still navigate directly to /security/acls/create, which was removed in this PR. Fix this →

2. Non-reactive role membership lookup [use-principal-permissions.ts:105]

rolesApi.roleMembers is accessed as a plain property inside useMemo, not via a store hook. This means the memo won't re-run reactively when memberships change — the permissions list will be stale until the page remounts.

// use-principal-permissions.ts:105
for (const [roleName, members] of rolesApi.roleMembers) {  // ← not reactive

3. Dead code [user-details.tsx:48]

The return value of formatMechanism(...) is computed but immediately discarded. This is almost certainly a bug — the mechanism label was supposed to be used somewhere (e.g., displayed in the user info section).

// user-details.tsx:48
formatMechanism(currentUser?.mechanism); // result is unused

⚠️ Code Quality

4. Raw <input> elements instead of registry <Input>

Two places use raw <input> with copy-pasted Tailwind classes instead of the <Input> registry component, which goes against the project's code standards (new code MUST use Registry components):

• permissions-list-tab.tsx:368 — search input
• add-acl-dialog.tsx:199 — group principal input (inconsistent with the <Combobox> used for the User type)

Fix this →

5. Addressing existing reviewer comments

• role-create-dialog.tsx:58 (@malinskibeniamin) — Agreed, use ConnectError.from(error) to get structured gRPC error info (code, message) instead of falling back to the raw JS error message:

  // current
  toast.error(`Failed to create role: ${err instanceof Error ? err.message : String(err)}`);
  // suggested
  const connectErr = ConnectError.from(err);
  toast.error(`Failed to create role: ${connectErr.message}`);

• role-detail-page.tsx:100 (@malinskibeniamin) — The toast should mention the removed member name for clarity. Suggest: `User "${parsePrincipal(memberPrincipal).name}" removed from role`

• acls-card.tsx:204 (@malinskibeniamin) — <h2 className="font-semibold text-base"> could use the <Typography> component if not mixing Chakra/Tailwind. Given this file is pure Tailwind, this is a low-priority style preference.

• description-with-help.tsx (@malinskibeniamin) — The Drawer-for-all-help-text pattern is a valid concern. For short single-line hints a tooltip (title attribute or <Tooltip>) is less disruptive. Consider accepting a compact?: boolean prop or rendering a <Tooltip> when children is a short string. For the current usages (paragraphs of text), the Drawer is appropriate.

• permissions-list-tab.tsx:100 (@malinskibeniamin) — The summaryText IIFE is readable but verbose. The pluralize helper (if it exists in this codebase) would clean it up. Worth a follow-up.

• permissions-list-tab.tsx:244 (@malinskibeniamin) — Using <Empty> from the registry for the "No principals" empty state would be consistent with other pages. Worth a follow-up.

• add-acl-dialog.tsx:256 (@malinskibeniamin) — The reviewer asks "Is this a card?" — looking at the code, the context is the pattern-type segmented control (<div className="inline-flex h-10 w-full items-center...">). This is not a card, it's a tab/toggle group. Not an issue.

🔍 Minor Observations

• SecurityTabsNav — the Roles tab shows for all non-serverless envs but is disabled based on featureRolesApi. The disabled tab renders but isn't clickable — consider hiding it entirely when disabled to reduce confusion.

• acls-card.tsx — row IDs are assigned with a mutable rowCounter that resets on each render. Since setSelected(new Set()) is called after deletions, this doesn't cause visible bugs, but it's fragile. A stable key derived from the ACL fields (principal + resourceType + operation + ...) would be more robust and could also protect against duplicate-selection bugs.

• user-create-dialog.tsx — the dialog doesn't reset step back to 'form' after the dialog is closed while at 'confirmation'. Re-opening will briefly show the confirmation screen before handleClose fires. Add setStep('form') to handleClose.